Fix the bug for restoring data with duplicated page id #4889
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
do-not-merge/needs-linked-issue
release-note-none
|
/run-all-tests |
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
|
/run-all-tests |
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
flowbehappy
reviewed
May 14, 2022
dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp
Outdated
Show resolved
Hide resolved
|
/rebuild |
|
/run-all-tests |
JaySon-Huang
commented
May 16, 2022
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
|
/run-all-tests |
JaySon-Huang
force-pushed
the
compact_when_restore
branch
from
670a4c7
to
cf8409b
Compare
This reverts commit cdaea05.
JaySon-Huang
force-pushed
the
compact_when_restore
branch
from
cf8409b
to
8e84bc3
Compare
JaySon-Huang
commented
May 16, 2022
Comment on lines
+943
to
+964
| do | ||
| { | ||
| // Can't find any entries in current ns_id | ||
| if (iter->first.high != ns_id) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| // Check and return whether this id is visible, otherwise continue to check the previous one. | ||
| if (iter->second->isVisible(UINT64_MAX - 1)) | ||
| { | ||
| return iter->first.low; | ||
| } | ||
|
|
||
| // Current entry/ref/external is deleted and there are no entries before it. | ||
| if (iter == mvcc_table_directory.begin()) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| iter--; | ||
| } while (true); |
|
/run-all-tests |
flowbehappy
reviewed
May 16, 2022
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
flowbehappy
reviewed
May 16, 2022
ti-chi-bot
removed
the
status/can-merge
|
/merge |
|
@JaySon-Huang: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
This pull request has been accepted and is ready to merge. Commit hash: 055e4bb
|
ti-chi-bot
added
the
status/can-merge
ti-chi-bot
removed
the
status/can-merge
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
|
/merge |
|
@JaySon-Huang: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
This pull request has been accepted and is ready to merge. Commit hash: cb236c1
|
ti-chi-bot
added
the
status/can-merge
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
|
/run-integration-test |
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
|
/merge |
|
@JaySon-Huang: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
|
@JaySon-Huang: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
/rebuild |
|
/run-integration-test |
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
12 tasks
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #3594
Problem Summary:
#4872
#4849
We fix those two bugs with previous PRs, but those changes introduce new misbehavior for TiFlash. Find a better way to fix those bugs.
What is changed and how it works?
Fix reusing same page_id for different kind by:
VersionedPageEntries:: createNewEntry/createNewExternal/createNewRef/createNewExternal/createNewRefrun into invali stateOther changes:
PageStorage->getMaxIdPageStorage->getMaxIdnow ignore the invisible idmeta_max_id, usePageEntry.isValid()to check whether page 1 is existed or nottiflash/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp
Lines 246 to 247 in 3e46ac2
Check List
Tests
Side effects
Documentation
Release note